Include request/response info with Abort error #5639
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pixelhandler
commented
Sep 14, 2018
addon/adapters/rest.js
Outdated
| @@ -1228,7 +1228,8 @@ function ajaxError(adapter, payload, requestData, responseData) { | |||
| } else if (responseData.textStatus === 'timeout') { | |||
| error = new TimeoutError(); | |||
| } else if (responseData.textStatus === 'abort' || responseData.status === 0) { | |||
| error = new AbortError(); | |||
| // TODO REVIEW should we use an adapter method for this, e.g. like handleResponse? | |||
There was a problem hiding this comment.
Any ideas on this topic, should the ability to handle the abort error live on the adapter?
try {
error = adapter.handleAbort(
responseData.status,
responseData.headers,
responseData.errorThrown || '',
requestData
);
} catch (e) {
error = e;
}
addon/adapters/rest.js
Outdated
| function handleAbort(requestData, responseData) { | ||
| let msg = `Request failed: ${requestData.method} ${requestData.url} ${requestData.errorThrown || ''}`; | ||
| let errors = [{ title: 'Adapter Error', detail: msg.trim(), status: responseData.status }]; | ||
| return new AbortError(errors); |
There was a problem hiding this comment.
Having this error with the url, and error response if available would be much better.
Currently we only use new AbortError();
pixelhandler
force-pushed
the
include-request-response-info-with-abort-error
branch
from
5e8183f to
c76d07c
Compare
pixelhandler
commented
Sep 14, 2018
| let { status } = responseData; | ||
| let msg = `Request failed: ${method} ${url} ${errorThrown || ''}`; | ||
| let errors = [{ title: 'Adapter Error', detail: msg.trim(), status }]; | ||
| return new AbortError(errors); |
There was a problem hiding this comment.
Having this error with the url, and error response if available would be much better.
Currently we only use new AbortError();
- Add test for capturing error details
pixelhandler
force-pushed
the
include-request-response-info-with-abort-error
branch
from
755bc27 to
c0eb4d8
Compare
|
Thanks so much! |
|
Likewise thank you @runspired |
|
@runspired any chance this one can be merged into an LTS? In v3.5 release...
Our app is not able to upgrade to 3.5 and just dies on loading, We're on 3.4 |
pixelhandler
deleted the
include-request-response-info-with-abort-error
branch
pixelhandler
pushed a commit
to pixelhandler/data
that referenced
this pull request
Oct 15, 2018
…ponse-info-with-abort-error Include request/response info with Abort error
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently the
AbortErrordoes not have any context about the request/response.This PR adds the request method, url and error if available...
The problem is that when you have this error there is no related info captured to identify why the adapter aborted. For example, using a service like bugsnag or raygun, the error may not have any information to help reproduce the problem in your application.